-
Notifications
You must be signed in to change notification settings - Fork 181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RoundRobinLoadBalancer fix connection leak during graceful closure #2450
Conversation
@@ -640,12 +663,12 @@ previous, new ConnState(newList, newState))) { | |||
LOGGER.trace("Load balancer for {}: added a new connection {} to {} after {} attempt(s).", | |||
targetResource, connection, this, addAttempt); | |||
// Instrument the new connection so we prune it on close | |||
connection.onClosing().beforeFinally(() -> { | |||
connection.onClose().beforeFinally(() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while this is preferred to remove connections from the selection process we still need to manage the lifecycle of the connections. we can use connection.onClosing
in the future to disable selection eligibility but the connection by adding extra state or another collection but this PR prioritizes correctness and we can followup to optimize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good findings, thanks!
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Outdated
Show resolved
Hide resolved
servicetalk-loadbalancer/src/main/java/io/servicetalk/loadbalancer/RoundRobinLoadBalancer.java
Show resolved
Hide resolved
Motivation: RoundRobinLoadBalancer clears state of all hosts and each host clears all connections when closing. However a common pattern is to initiate gracefulClosure with a timeout, and then force a full close after a timeout interval. In this scenario RRLB has discarded the reference to connections and will not full close the connections. Modifications: - asyncCloseable needs to keep state about what hosts are being closed and each host can't dereference connections until they are fully closed. - switch back to using onClose to cleanup the connection list as otherwise we may leak connections that have started graceful closure and never finished.
7be1407
to
ec4245d
Compare
ec4245d
to
aa0def6
Compare
Motivation:
RoundRobinLoadBalancer clears state of all hosts and each host clears all connections when closing. However a common pattern is to initiate gracefulClosure with a timeout, and then force a full close after a timeout interval. In this scenario RRLB has discarded the reference to connections and will not full close the connections.
Modifications: